[Workspace]Fix error toasts in sample data page #8842

wanglam · 2024-11-11T08:00:56Z

Description

This PR addresses the issue of error toasts appearing on the sample data page when installing or uninstalling sample data. The root cause was that the user did not have permission to update UI settings at the workspace level. The changes in this PR include:

Remove the default index UI setting first if deleting an index pattern in a specific workspace.
Remove index patterns first when uninstalling sample data.
Disable the behavior of updating the default index pattern UI setting when installing or uninstalling sample data inside a workspace.

Screenshot

No UI Changes

Testing the changes

Clone the branch and run yarn osd bootstrap --single-version loose.

Add the following configurations to config/opensearch_dashboards.yml:

savedObjects.permission.enabled: true
workspace.enabled: true
uiSettings:
  overrides:
    'home:useNewHomePage': true

Run yarn start --no-base-path.
Log in with the admin user and create a new workspace named "test1".
Visit the sample data page of the "test1" workspace.
Click the "Add data" and "Remove" buttons on the sample data cards to ensure you can install and uninstall sample data without any issues.
Create a new user with the "admin" backend_role in the "Internal users" page.
Assign the "Read and write" access level to the new user inside the "test1" workspace.
Open an incognito window and log in with the new user.
Visit the sample data page of the "test1" workspace and click the "Add data" button. Verify that no error toasts appear.
Switch back to the admin user tab, visit the same workspace, and set the "flights" sample data as the default index pattern in the index patterns page.
Switch to the new user tab and click the "Remove" button on the "flights" sample data card. Verify that an error toast appears, similar to the provided screenshot.

13. Click the "Remove" buttons on other sample data cards and ensure no error toasts appear. 14. Switch to the admin user tab and set the new user as an "Admin" of the "test1" workspace. 15. Switch to the new user tab and click the "Remove" button on the "flights" sample data card. Verify that no error toasts appear. 16. Test with the workspace disabled (for regression testing): - Remove the newly added configurations. - Visit the home page and click the "Add sample data" button. - Click the "Add data" buttons. Sample data should be installed without error toasts. - Click the "Remove" buttons. Sample data should be uninstalled without error toasts.

Changelog

fix: [Workspace]Fix error toasts in sample data page

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

Signed-off-by: Lin Wang <[email protected]>

codecov · 2024-11-11T08:32:03Z

Codecov Report

Attention: Patch coverage is 90.90909% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.90%. Comparing base (b31206a) to head (bddcd71).

Files with missing lines	Patch %	Lines
...me/server/services/sample_data/routes/uninstall.ts	83.33%	1 Missing and 1 partial ⚠️
...ed_objects/workspace_ui_settings_client_wrapper.ts	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8842      +/-   ##
==========================================
+ Coverage   60.88%   60.90%   +0.02%     
==========================================
  Files        3802     3802              
  Lines       91083    91114      +31     
  Branches    14383    14391       +8     
==========================================
+ Hits        55455    55496      +41     
+ Misses      32086    32074      -12     
- Partials     3542     3544       +2

Flag	Coverage Δ
Linux_1	`29.04% <94.11%> (+0.01%)`	⬆️
Linux_2	`56.38% <ø> (ø)`
Linux_3	`37.90% <ø> (ø)`
Linux_4	`29.04% <87.50%> (+0.03%)`	⬆️
Windows_1	`29.06% <94.11%> (+0.01%)`	⬆️
Windows_2	`56.34% <ø> (ø)`
Windows_3	`37.90% <ø> (ø)`
Windows_4	`29.04% <87.50%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This reverts commit b92b333. Signed-off-by: Lin Wang <[email protected]>

Signed-off-by: Lin Wang <[email protected]>

…admin

SuZhou-Joe · 2024-11-28T09:21:07Z

src/plugins/home/public/application/sample_data_client.js

@@ -41,11 +41,16 @@ export async function listSampleDataSets(dataSourceId) {
  return await getServices().http.get(sampleDataUrl, { query });
 }

+const isWorkspaceEnabled = () => {


I am thinking if it is workspaceEnabled or permissionControlEnabled.

SuZhou-Joe · 2024-11-28T09:27:51Z

src/plugins/workspace/server/saved_objects/workspace_ui_settings_client_wrapper.ts

@@ -144,6 +148,59 @@ export class WorkspaceUiSettingsClientWrapper {
      return wrapperOptions.client.update(type, id, attributes, options);
    };

+    const deleteUiSettingsWithWorkspace = async (


Suggested change

const deleteUiSettingsWithWorkspace = async (

const deleteWithDefaultIndexPatternCheck = async (

Signed-off-by: Lin Wang <[email protected]>

…admin

Hailong-am · 2024-11-29T07:54:18Z

src/plugins/home/public/application/sample_data_client.js

 export async function installSampleDataSet(id, sampleDataDefaultIndex, dataSourceId) {
  const query = buildQuery(dataSourceId);
  await getServices().http.post(`${sampleDataUrl}/${id}`, { query });

-  if (getServices().uiSettings.isDefault('defaultIndex')) {
+  if (!isWorkspaceEnabled() && getServices().uiSettings.isDefault('defaultIndex')) {


can we add a unit test to cover this condition check?

Hailong-am · 2024-11-29T07:58:52Z

src/plugins/home/public/application/sample_data_client.js

  if (
+    !isWorkspaceEnabled() &&
    !uiSettings.isDefault('defaultIndex') &&
    uiSettings.get('defaultIndex') === sampleDataDefaultIndex


can we add a comment here, workspace_ui_settings_client_wrapper will handle default index pattern reset?

Hailong-am · 2024-11-29T08:20:32Z

src/plugins/workspace/server/saved_objects/workspace_ui_settings_client_wrapper.ts

+      const defaultIndexPatternWorkspaces = workspaceObjects.saved_objects.filter(
+        ({ attributes }) => attributes?.uiSettings?.[DEFAULT_INDEX_PATTERN_UI_SETTINGS_ID] === id
+      );
+      try {
+        await this.getWorkspaceTypeEnabledClient(wrapperOptions.request).bulkUpdate(
+          defaultIndexPatternWorkspaces.map((savedObject) => ({


can we check defaultIndexPatternWorkspaces is not empty first?

also will bulkUpdate an empty list throw errors?

The bulkUpdate won't throw errors for empty list. But you're right, we can add a empty check logic for it.

Signed-off-by: Lin Wang <[email protected]>

…admin

ruanyl · 2024-12-02T09:15:19Z

src/plugins/home/server/services/sample_data/routes/uninstall.ts

+       * This is necessary because default index patterns cannot be deleted
+       * when a workspace is enabled and the user lacks the required permissions.


This is necessary because default index patterns cannot be deleted
when a workspace is enabled and the user lacks the required permissions.

I might miss something here, do we have special check on index pattern when deleting?

In this PR, we will delete index patterns of sample data first when user want to uninstall sample data. So there are different cases when user try to uninstall sample data.

Case 1: Index pattern is default index pattern and user is dashboards admin or workspace admin, Index pattern can be removed

Case 2: Index pattern is default index pattern and user is workspace read and write and workspace read only, index pattern can't be removed

Case 3: Index pattern is not default index pattern and user is dashboards admin, workspace admin or workspace read & write, index pattern can be removed

Case 4: Index pattern is not default index pattern and user is workspace read only, index pattern can't be removed
This PR will enhance case 1 and case 2, will try to remove ui settings first when deleted index patterns are default index pattern.

ruanyl · 2024-12-02T09:35:05Z

src/plugins/workspace/server/saved_objects/workspace_ui_settings_client_wrapper.ts

@@ -144,6 +148,61 @@ export class WorkspaceUiSettingsClientWrapper {
      return wrapperOptions.client.update(type, id, attributes, options);
    };

+    const deleteWithDefaultIndexPatternCheck = async (


I'm a bit concern that we're over complicating this saved object wrapper, it's intended to be a wrapper for config type of object, but now we're doing an implicit cascading updating when a index-pattern is deleted.
Could you please elaborate why this change is necessary?

You are right, we can keep the default index pattern validation logic in the ui setting PR when trying to delete index patterns. Then it will throw an error if a user tries to remove a default index pattern. The benefits of leaving the cascading updating logic were that the user doesn't need to call the UI setting separately. I'm open to this. After this change, the user will get an error when trying to uninstall sample data and the index pattern is a default index pattern. Do you think if we should remove the default index patterns ui setting before sample data uninstall?

Fix error toasts in sample data page

b92b333

Signed-off-by: Lin Wang <[email protected]>

github-actions bot added the distinguished-contributor label Nov 11, 2024

Changeset file for PR opensearch-project#8842 created/updated

f199290

wanglam marked this pull request as ready for review November 11, 2024 09:25

SuZhou-Joe previously approved these changes Nov 12, 2024

View reviewed changes

SuZhou-Joe added backport 2.x workspace labels Nov 12, 2024

wanglam marked this pull request as draft November 21, 2024 06:03

wanglam added 4 commits November 22, 2024 12:09

Revert "Fix error toasts in sample data page"

d76a356

This reverts commit b92b333. Signed-off-by: Lin Wang <[email protected]>

Update default index pattern setting before index pattern deleted

bea08d1

Signed-off-by: Lin Wang <[email protected]>

Remove index pattern first when uninstall sample data inside workspace

7cb72a2

Signed-off-by: Lin Wang <[email protected]>

Set default index pattern when workspace disabled

9119767

Signed-off-by: Lin Wang <[email protected]>

wanglam dismissed SuZhou-Joe’s stale review via 9119767 November 22, 2024 04:10

Merge branch 'main' into fix-default-index-setting-for-non-workspace-…

c156004

…admin

wanglam marked this pull request as ready for review November 22, 2024 06:28

wanglam requested a review from SuZhou-Joe November 22, 2024 10:05

Merge branch 'main' into fix-default-index-setting-for-non-workspace-…

a6e4ea4

…admin

SuZhou-Joe reviewed Nov 28, 2024

View reviewed changes

wanglam added 2 commits November 29, 2024 11:45

Refactor with deleteWithDefaultIndexPatternCheck

fa38231

Signed-off-by: Lin Wang <[email protected]>

Merge branch 'main' into fix-default-index-setting-for-non-workspace-…

80fbe13

…admin

Hailong-am reviewed Nov 29, 2024

View reviewed changes

Address PR comments and add more UTs

f537c6f

Signed-off-by: Lin Wang <[email protected]>

SuZhou-Joe approved these changes Dec 2, 2024

View reviewed changes

Merge branch 'main' into fix-default-index-setting-for-non-workspace-…

bddcd71

…admin

ruanyl reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Workspace]Fix error toasts in sample data page #8842

[Workspace]Fix error toasts in sample data page #8842

wanglam commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 11, 2024 •

edited

Loading

SuZhou-Joe Nov 28, 2024

SuZhou-Joe Nov 28, 2024

Hailong-am Nov 29, 2024

Hailong-am Nov 29, 2024

Hailong-am Nov 29, 2024 •

edited

Loading

wanglam Dec 2, 2024

ruanyl Dec 2, 2024

wanglam Dec 2, 2024

ruanyl Dec 2, 2024

wanglam Dec 2, 2024

	const deleteUiSettingsWithWorkspace = async (
	const deleteWithDefaultIndexPatternCheck = async (

		* This is necessary because default index patterns cannot be deleted
		* when a workspace is enabled and the user lacks the required permissions.

[Workspace]Fix error toasts in sample data page #8842

Are you sure you want to change the base?

[Workspace]Fix error toasts in sample data page #8842

Conversation

wanglam commented Nov 11, 2024 • edited Loading

Description

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Nov 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hailong-am Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanglam commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 11, 2024 •

edited

Loading

Hailong-am Nov 29, 2024 •

edited

Loading